-
Notifications
You must be signed in to change notification settings - Fork 9.4k
#40216-Replacing nested foreach with array union (+) to reduce time complexity #40217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 2.4-develop
Are you sure you want to change the base?
#40216-Replacing nested foreach with array union (+) to reduce time complexity #40217
Conversation
Hi @senthilengg. Thank you for your contribution!
Allowed build names are:
You can find more information about the builds here For more details, review the Code Contributions documentation. |
Failed to run the builds. Please try to re-run them later. |
@magento run unit Tests |
@magento run Database Compare,Functional Tests CE,Functional Tests EE,Functional Tests B2B,Integration Tests,Magento Health Index,Sample Data Tests CE,Sample Data Tests EE,Sample Data Tests B2B,Static Tests,WebAPI Tests,Semantic Version Checker |
@magento run Functional Tests CE,Static Tests,WebAPI Tests |
@magento run Functional Tests EE,Functional Tests B2B,Integration Tests |
…left array untouched
@magento run Functional Tests CE |
@magento run WebAPI Tests |
@magento run WebAPI Tests |
@magento run WebAPI Tests |
@magento run all tests |
@magento run all tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @senthilengg,
Thank you for your contribution!
Please have a look into the below review comments.
* @return $this | ||
* @throws LocalizedException | ||
*/ | ||
protected function _setItemAttributeValue($valueInfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method signature behavior has changed. Any plugin/extension overriding _setItemAttributeValue()
will break.
Recommendations:
- Create a new method
_setItemAttributeValues()
(plural) for batch processing - Keep _setItemAttributeValue() (singular) unchanged for BC
- Add an automated test for the new method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@engcom-Hotel I think you didn't notice its a protected function in an abstract class. Let me know if you still think we need to introduce new function for other reasons. I couldn't think of a scenario someone wants to override the under the hood Eav Object processing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @senthilengg,
Thanks for your reply!
but I am still thinks that this is the BiC change. Please go through with the below link:
and let me know If I missed anything here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@engcom-Hotel Thank you for sharing the link. Its done!
__('A header row is missing for an attribute. Verify the header row and try again.') | ||
); | ||
} | ||
// Run only once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: If it "runs only once" why is there a foreach loop? This suggests _itemsById[$entityId]
contains multiple objects, contradicting the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@engcom-Hotel Without this loop during my manual functional testing and unit testing is working well and passed, I have tested with configurable, bundle and simple products. However without the loop its failing WebAPI Graphql MTF test so there could be a scenario the test script except this to run in a loop but I couldn't reproduce myself. Tried with Xdebug profile as well during the manual test.
To give you another perspective I couldn't think of a scenario where this _itemsById
contains more than one product object under the same Id.
_itemsById
was set here
if (isset($this->_itemsById[$object->getId()])) { |
$object->getId()
here being the entity ID from the DB can't be a duplicated. So to me it looks like a legacy code sitting there and push the object to the _itemsById
array if the same id exists. And this is the reason why MTF Web API test also expecting something, similar however in real world, I couldn't think of a scenario happens like this and thats the reason for my comment that it will run only once.
In any case if you feel its better to remove the please do so. Because if the object contains multiple it should have affected the stores much more since it would have run O( abc) c being the number of objects inside _itemsById
And Thank you so much for the review. Appreciate it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your analysis @senthilengg. I agrees with your point. But I have suggestion below to change the comment as below:
// _itemsById[$entityId] is always an array (typically with one element)
// Foreach handles edge cases where multiple objects share the same entity ID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@engcom-Hotel Its done!
* @return $this | ||
* @throws LocalizedException | ||
*/ | ||
protected function _setItemAttributeValue($valueInfo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @senthilengg,
Thanks for your reply!
but I am still thinks that this is the BiC change. Please go through with the below link:
and let me know If I missed anything here.
__('A header row is missing for an attribute. Verify the header row and try again.') | ||
); | ||
} | ||
// Run only once |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your analysis @senthilengg. I agrees with your point. But I have suggestion below to change the comment as below:
// _itemsById[$entityId] is always an array (typically with one element)
// Foreach handles edge cases where multiple objects share the same entity ID
…cate _setItemAttributeValue
@magento run Unit Tests, WebAPI Tests, Static Tests |
@magento run all tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @senthilengg for making the changes.
Failed tests seems flaky to me, hence approving this PR.
Description (*)
[product_id => [attribute_code => value, attribute_code1 => value1]]
_setItemAttributeValue
only once per$selectGroups
instead of per$selectGroups
, per attribute and per productFixed Issues (if relevant)
Manual testing scenarios (*)
Result
Without this PR - Too many calls to _setItemAttributeValue function

With PR - Only one call to _setItemAttributeValue function

Additional Questions
Since array_replace is memory intensive for large array would it benefit over all or not ?
array union (+) seems to be memory optimised and used here